fix: keep UDS peer failures structured#375
Merged
claude-code-best merged 4 commits intomainfrom Apr 27, 2026
Merged
Conversation
CodeRabbit and Claude cross-review identified that timeout and raw peer connection failures should share one observable error contract. UDS peer failures now use UdsPeerConnectionError consistently, and connectToPeer hands the socket lifecycle back to the caller after a successful connection instead of retaining an internal timeout or error listener. The tests cover the real socket paths with capability files, timeout behavior, connection failure structure, post-connect listener handoff, AgentSummary rescheduling observations, and platform-specific mailbox directory errno handling. Constraint: Preserve the 5000ms production timeout default while allowing tests to exercise timeout paths quickly. Rejected: Suppress CodeRabbit warnings in tests | would hide the real timeout/error contract gap. Rejected: Keep connectToPeer post-connect error listener | it would silently swallow caller-owned socket errors. Confidence: high Scope-risk: narrow Directive: Keep UDS send/connect timeout and socket-error paths on the same structured peer error contract. Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/teammateMailbox.test.ts Tested: bunx tsc --noEmit --pretty false Tested: bun run lint Tested: bun run test:all Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage Tested: bun run build Tested: bun run build:vite Tested: omx ask claude simplify review artifact .omx/artifacts/claude-review-only-cross-check-for-pr-374-on-branch-codex-codecov-r-2026-04-27T08-17-47-309Z.md Tested: omx ask claude security review artifact .omx/artifacts/claude-security-review-cross-check-for-pr-374-current-working-tree--2026-04-27T08-26-54-079Z.md Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors UDS client to accept an onSocketError callback and configurable timeout; centralizes pre-connect failure handling. Updates tests: AgentSummary log checks use substring matching, teammateMailbox errno expectation is platform-aware, and udsMessaging tests expand timeout and add connectToPeer lifecycle/error expectations. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant UdsClient
participant Socket
participant Timeout
Caller->>UdsClient: connectToPeer(socketPath, onSocketError, timeoutMs)
UdsClient->>Socket: create socket, socket.connect(socketPath)
UdsClient->>Timeout: start timeout timer
par pre-connect error
Socket-->>UdsClient: 'error' (pre-connect)
UdsClient->>Socket: remove listeners, destroy()
UdsClient-->>Caller: reject UdsPeerConnectionError(socketPath, cause)
and timeout expires
Timeout-->>UdsClient: timeout expiry
UdsClient->>Socket: remove listeners, destroy()
UdsClient-->>Caller: reject UdsPeerConnectionError(socketPath, timeout)
end
opt connect success
Socket-->>UdsClient: 'connect'
UdsClient->>Timeout: clear()
UdsClient->>Socket: remove internal pre-connect listeners
UdsClient->>Socket: attach onSocketError handler
UdsClient-->>Caller: resolve Socket
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/udsClient.ts (1)
271-297: Clean handoff of socket lifecycle to caller.The shared
failpath with thesettledguard covers both the'error'and timeout races correctly: once'connect'resolves,setTimeout(0)disables the inactivity timer (so the timeout callback can no longer fire) andoff('error', fail)detaches the internal listener so caller-owned errors aren't swallowed. The reciprocal path (timeout/error →faildestroys + rejects) is also single-shot viasettled.One consequence worth highlighting (consistent with the PR's stated design decision): between
resolve(conn)and the caller attaching its own'error'handler, the socket has no error listener, and an unhandled'error'event would surface as an uncaught exception. Callers must attach anerrorhandler synchronously upon receiving the socket. Both current call sites in the new tests satisfy this implicitly via the test scaffolding, but a brief note in the JSDoc would make the contract explicit for future callers.📝 Suggested doc tightening
/** * Connect to a peer and return the raw socket for bidirectional communication. - * The caller is responsible for managing the connection lifecycle. + * The caller is responsible for managing the connection lifecycle, including + * attaching an `'error'` listener immediately upon receiving the socket — + * this function detaches its internal error listener on successful connect + * so caller-owned errors are not silently swallowed. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/udsClient.ts` around lines 271 - 297, The connectToPeer function hands a raw Socket (created via createConnection) to the caller once 'connect' fires and removes the internal 'error' listener, which can leave the socket without any error handler until the caller attaches one; update the JSDoc for connectToPeer to explicitly document this lifecycle contract: state that callers must synchronously attach an 'error' listener immediately after the Promise resolves (or use try/catch around await and attach before any I/O) because unhandled 'error' events on the socket will throw, and reference connectToPeer, createConnection, and UdsPeerConnectionError to make the expectation clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/__tests__/udsMessaging.test.ts`:
- Around line 322-358: The test 'connectToPeer leaves connected socket lifecycle
to the caller' uses connectToPeer(path, 50) with a 50ms timeout which is too
tight and causes flakiness on slow CI; update the call to use a larger timeout
(e.g., 1000ms) so the test still checks post-connect lifecycle while avoiding
transient UdsPeerConnectionError failures from slow connects, i.e., change the
literal 50 in the connectToPeer invocation in this test to 1000 (referencing the
connectToPeer import from ../udsClient.js and the test block named
'connectToPeer leaves connected socket lifecycle to the caller').
---
Nitpick comments:
In `@src/utils/udsClient.ts`:
- Around line 271-297: The connectToPeer function hands a raw Socket (created
via createConnection) to the caller once 'connect' fires and removes the
internal 'error' listener, which can leave the socket without any error handler
until the caller attaches one; update the JSDoc for connectToPeer to explicitly
document this lifecycle contract: state that callers must synchronously attach
an 'error' listener immediately after the Promise resolves (or use try/catch
around await and attach before any I/O) because unhandled 'error' events on the
socket will throw, and reference connectToPeer, createConnection, and
UdsPeerConnectionError to make the expectation clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eaa62c50-57f8-41c1-9670-5ae7d5df699c
📒 Files selected for processing (4)
src/services/AgentSummary/__tests__/agentSummary.test.tssrc/utils/__tests__/teammateMailbox.test.tssrc/utils/__tests__/udsMessaging.test.tssrc/utils/udsClient.ts
CodeRabbit's #375 pass found that connectToPeer now correctly hands socket errors to the caller, but the JSDoc needed to spell out that contract. The lifecycle test also uses a less brittle post-connect timeout so slow CI does not turn the ownership check into a connection-speed race. Constraint: The raw socket API intentionally detaches its internal listener after successful connect so caller-owned errors are not swallowed. Rejected: Keep the test timeout at 50ms | it tests scheduler speed instead of socket lifecycle ownership. Confidence: high Scope-risk: narrow Directive: connectToPeer callers must attach their own error listener immediately after awaiting the socket. Tested: bun test src/utils/__tests__/udsMessaging.test.ts Tested: bunx tsc --noEmit --pretty false Tested: bun run lint Tested: git diff --check Tested: bun run test:all Not-tested: GitHub-hosted CodeRabbit refresh until pushed.
CodeRabbit and Claude review found that documenting caller-owned raw socket errors still left a Promise handoff window and a stale timeout-listener risk. The peer connection API now requires a caller error handler and installs it before resolving, while cleanup removes internal error and timeout listeners on every path. Constraint: Keep the fix precise to PR #375 review feedback and avoid warning suppression or fallback behavior. Rejected: Leave the behavior documented only | still permits an unhandled socket error window between resolve and caller listener attachment. Rejected: Keep a no-op internal error listener | would silently swallow caller-owned socket errors. Confidence: high Scope-risk: narrow Directive: Do not add raw connectToPeer callers without providing a real onSocketError handler and capability handshake. Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts Tested: bunx tsc --noEmit --pretty false Tested: bun run lint Tested: bun run test:all Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage Tested: bun run build Tested: bun run build:vite Tested: bun audit Not-tested: Manual external ACP peer runtime beyond repository tests.
The raw socket handoff no longer needs Socket#setTimeout; an ordinary connection deadline keeps the timeout behavior while avoiding an internal socket timeout listener that has no reliable UDS integration path to exercise. Constraint: Keep Codecov coverage honest without adding ignore pragmas, mocks, or fallback suppression. Rejected: c8 ignore on the timeout listener | hides the uncovered branch instead of simplifying the lifecycle. Rejected: keep Socket#setTimeout listener | leaves a socket listener lifecycle to manage for a connect-only deadline. Confidence: high Scope-risk: narrow Directive: Keep connectToPeer errors caller-owned via onSocketError and reject pre-connect failures with UdsPeerConnectionError. Tested: bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts Tested: bunx tsc --noEmit --pretty false Tested: bun run lint Tested: bun test src/utils/__tests__/udsMessaging.test.ts --coverage --coverage-reporter lcov --coverage-dir coverage-uds Tested: bun run test:all Tested: bun test --coverage --coverage-reporter lcov --coverage-dir coverage Tested: bun run build Tested: bun run build:vite Tested: bun audit Not-tested: Manual external ACP peer runtime beyond repository tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up after #374 was squash-merged before the final review fixes landed.
This PR carries only the final, clean delta on top of current
main:UdsPeerConnectionError.Verification
bun test src/utils/__tests__/udsMessaging.test.ts src/services/AgentSummary/__tests__/agentSummary.test.ts src/utils/__tests__/teammateMailbox.test.tsbunx tsc --noEmit --pretty falsebun run lintbun run test:allbun test --coverage --coverage-reporter lcov --coverage-dir coveragebun run buildbun run build:vite.omx/artifacts/claude-review-only-cross-check-for-pr-374-on-branch-codex-codecov-r-2026-04-27T08-17-47-309Z.md.omx/artifacts/claude-security-review-cross-check-for-pr-374-current-working-tree--2026-04-27T08-26-54-079Z.mdSummary by CodeRabbit
Tests
Refactor